Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for flux bias with weight windows in multigroup mode #3202

Merged
merged 9 commits into from
Jan 24, 2025

Conversation

pshriwise
Copy link
Contributor

@pshriwise pshriwise commented Nov 20, 2024

Description

@jtramm and I were applying some weight windows in multigroup mode yesterday and while variance was being reduced as expected, there was a bias being introduced when weight windows were applied.

It turned out this is caused by the apply_weight_windows function call occurring before sample_reaction in the collision_mg method (here). Once this was changed, the bias disappeared and the weight windows worked as expected.

When particles are split before the reaction is sampled not all of the weight is applied to the sampled reaction. In the case of an absorption, for example, some of the particle's original weight will go into split particles that are revived later before the reaction occurs and the particle weight is set to zero, removing that weight from the problem. The split particles in the secondary bank will then contribute to the flux upon revival with weight that should have been removed due to the absorption reaction. When this happens enough, the effect is a bias as though absorption reactions aren't occurring as often as they should.

I've added a test here that checks bias in a simple problem with generated weight windows. Without the fix in physics_mg.cpp made here, the test fails due to a small bias in the total flux (evidence of failure here). This bias becomes more pronounced as more particles are run -- the test runs enough to make it measureable.

Thanks @jtramm for discovering this!

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@pshriwise pshriwise requested a review from nelsonag as a code owner November 20, 2024 14:57
Copy link
Contributor

@paulromano paulromano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pshriwise! Turned out the test failure was caused by the merge commit because we had already merged #3199 on develop, which meant that the merge commit had two calls to apply_weight_windows, one before sample_reaction and one after. Once I explicitly merged develop and got rid of the first one, that fixed the failing test (locally).

@paulromano paulromano enabled auto-merge (squash) January 24, 2025 16:53
@pshriwise
Copy link
Contributor Author

Excellent! Thank you for sorting that out.

@paulromano paulromano merged commit 7089780 into openmc-dev:develop Jan 24, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants